Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#437 Expose an event to skip suite errors #495

Merged
merged 12 commits into from
Feb 26, 2024

Conversation

NAVFreak
Copy link
Contributor

@NAVFreak NAVFreak commented Jan 19, 2024

Summary

This PR exposes an event to make it possible to skip an unwanted error when running a BCPT suit.
The idea was to expose an event i BCPTLine codeunit.

However, during the implementation I discovered that [BCPTLine codeunit has access internal] (https://github.com/microsoft/BCApps/blob/faeca5adabaa2e2b7644d744e21f9a1f97d1d971/src/Tools/Performance%20Toolkit/App/src/BCPTLine.Codeunit.al#L12).

To solve this I created a wrapper subscriber in BCPTTestSuite and calls another event that is public.

I hope this workaround is acceptable.

Work Item(s)

Fixes #437

Fixes AB#498190

Fixes AB#498190

Exposes an event to make it possible to skip errors from unhandled handler functions.
@NAVFreak NAVFreak requested a review from a team as a code owner January 19, 2024 06:47
@github-actions github-actions bot added AL: Tools From Fork Pull request is coming from a fork labels Jan 19, 2024
@JesperSchulz JesperSchulz added Linked Issue is linked to a Azure Boards work item and removed Linked Issue is linked to a Azure Boards work item labels Jan 30, 2024
@github-actions github-actions bot modified the milestone: Version 24.0 Jan 30, 2024
Copy link
Contributor

@grobyns grobyns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you wouldn't just raise the event directly on the facade. You don't need the 'wrapper' functions and without them, you also don't have a confusing naming issue...

@NAVFreak
Copy link
Contributor Author

NAVFreak commented Feb 2, 2024

@TKapitan , @grobyns I have now done the desired changes that you wanted

henrikfrovst
henrikfrovst previously approved these changes Feb 9, 2024
@NAVFreak
Copy link
Contributor Author

@ChethanT do you think this can go into next release? Meaning do you have time to merge it before the break? 😬

ChethanT
ChethanT previously approved these changes Feb 21, 2024
ChethanT
ChethanT previously approved these changes Feb 26, 2024
@JesperSchulz
Copy link
Contributor

We've got approval from @ChethanT, but the build is failing. Seems just to be some unused variables though, which should be easy to fix. One more round, and we can get this PR merged 😊

@JesperSchulz JesperSchulz enabled auto-merge (squash) February 26, 2024 12:56
@JesperSchulz JesperSchulz dismissed grobyns’s stale review February 26, 2024 12:56

Review comments have been addressed.

@JesperSchulz JesperSchulz merged commit 967d9f1 into microsoft:main Feb 26, 2024
14 checks passed
@JesperSchulz JesperSchulz added the Linked Issue is linked to a Azure Boards work item label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: Tools From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BC Idea]: BCPT Handled event in AddLogEntry
6 participants